Fix material readiness to gate on light texture readiness#18255
Fix material readiness to gate on light texture readiness#18255bghgary merged 5 commits intoBabylonJS:masterfrom
Conversation
SpotLight projection textures and IES profile textures were not gated by material readiness checks. When a projection texture was still loading, prepareLightSpecificDefines would set PROJECTEDLIGHTTEXTURE to false, the shader would compile without the projection effect, and the material would report isReady()=true. This caused scene.executeWhenReady() to fire before the projection texture loaded, producing incorrect rendering. Changes: - Add areLightTexturesReady() virtual method to Light base class - Override in SpotLight to check projection and IES textures - Track light texture readiness in PrepareDefinesForLight state - Propagate via defines._areLightTexturesReady in PrepareDefinesForLights - Gate isReadyForSubMesh() on light texture readiness in all materials: StandardMaterial, PBRBaseMaterial, BackgroundMaterial, OpenPBRMaterial, NodeMaterial, and Node Material light blocks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a readiness gap where SpotLight projection/IES textures could still be loading while materials (and therefore scene.executeWhenReady()) reported ready, causing rendering to proceed with shaders compiled without the projected/IES lighting path.
Changes:
- Add
Light.areLightTexturesReady()(defaulttrue) and override it inSpotLightto gate on projection/IES texture readiness. - Propagate a
lightTexturesReadyflag throughPrepareDefinesForLight(s)intodefines._areLightTexturesReady. - Update core materials (Standard, PBR, OpenPBR, Background, NodeMaterial) to return not-ready when
defines._areLightTexturesReady === false.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dev/core/src/Materials/standardMaterial.ts | Gates isReadyForSubMesh on defines._areLightTexturesReady. |
| packages/dev/core/src/Materials/PBR/pbrBaseMaterial.ts | Gates PBR readiness on defines._areLightTexturesReady. |
| packages/dev/core/src/Materials/PBR/openpbrMaterial.ts | Gates OpenPBR readiness on defines._areLightTexturesReady. |
| packages/dev/core/src/Materials/Node/nodeMaterial.ts | Gates NodeMaterial readiness on defines._areLightTexturesReady. |
| packages/dev/core/src/Materials/Node/Blocks/PBR/pbrMetallicRoughnessBlock.ts | Tracks per-light lightTexturesReady via PrepareDefinesForLight. |
| packages/dev/core/src/Materials/Node/Blocks/Dual/lightBlock.ts | Tracks per-light lightTexturesReady via PrepareDefinesForLight. |
| packages/dev/core/src/Materials/materialHelper.ts | Extends the typed PrepareDefinesForLight state contract to include lightTexturesReady. |
| packages/dev/core/src/Materials/materialHelper.functions.ts | Implements readiness propagation (state.lightTexturesReady → defines._areLightTexturesReady) and checks light.areLightTexturesReady(). |
| packages/dev/core/src/Materials/Background/backgroundMaterial.ts | Gates BackgroundMaterial readiness on defines._areLightTexturesReady. |
| packages/dev/core/src/Lights/spotLight.ts | Implements areLightTexturesReady() by checking projection + IES textures. |
| packages/dev/core/src/Lights/light.ts | Adds base areLightTexturesReady() API for light subclasses. |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18255/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18255/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18255/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
sebavan
left a comment
There was a problem hiding this comment.
Why using the defines for it ? I do not think we ever used them for it ?
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
Adds a unit test that demonstrates, on master, the bug that BabylonJS#18255 fixes: scene.isReady() incorrectly returns true while a SpotLight's projectionTexture is still loading. The test uses NullEngine + StandardMaterial + a mocked Texture whose isReady is pinned to false, then polls scene.isReady() while yielding so the NullEngine shader compile can settle. On master, scene.isReady() flips to true and the assertion fails. On the fix branch, it stays false and the assertion passes. This PR is expected to fail CI. It is the test-side companion to BabylonJS#18255. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a unit test that demonstrates, on master, the bug that BabylonJS#18255 fixes: scene.isReady() incorrectly returns true while a SpotLight's projectionTexture is still loading. The test uses NullEngine + StandardMaterial + a mocked Texture whose isReady is pinned to false, then polls scene.isReady() while yielding so the NullEngine shader compile can settle. On master, scene.isReady() flips to true and the assertion fails. On the fix branch, it stays false and the assertion passes. This PR is expected to fail CI. It is the test-side companion to BabylonJS#18255. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sProfileTexture Extends the regression test to cover: - The generic Light.areLightTexturesReady() contract (so future light types that add their own textures are gated automatically) - SpotLight.iesProfileTexture readiness (the other texture SpotLight.areLightTexturesReady checks) - SpotLight.projectionTexture readiness (unchanged) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18255/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18255/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18255/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
Addresses sebavan's objection to using defines for this readiness check, and the Copilot bot comments about multi-light accumulation, blocking semantics, and check ordering vs effect compilation. Changes: - Remove `defines._areLightTexturesReady` plumbing and the `state.lightTexturesReady` field from `PrepareDefinesForLight(s)`. - Add `AreLightsTexturesReady(scene, mesh, maxSimultaneousLights, disableLighting)` helper that iterates `mesh.lightSources` up to the material's cap and respects `lightsEnabled` / `disableLighting`, mirroring the light selection used by `PrepareDefinesForLights` / `BindLights`. - Gate readiness in each material (StandardMaterial, BackgroundMaterial, PBRBaseMaterial, OpenPBRMaterial, NodeMaterial) via the helper. For PBR/OpenPBR, the gate now runs BEFORE `_prepareEffect()` so we don't compile a throwaway shader while a projection/IES texture is loading. - Node blocks with an explicit `light`: PBRMetallicRoughnessBlock's existing `isReady` now also checks `this.light.areLightTexturesReady()`. LightBlock gets an `isReady` override and registers itself as a blocking block when it has an explicit light. - SpotLight.areLightTexturesReady now uses `isReadyOrNotBlocking()` to respect `BaseTexture.isBlocking` semantics, consistent with other material texture readiness checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
[Responded by Copilot on behalf of @bghgary]
Good point. Refactored in 476c12a — the defines plumbing is gone. Readiness is now checked directly in each material's This also cleans up three other issues the Copilot bot flagged: the multi-light accumulation hazard (defines getting overwritten by the last light), PBR/OpenPBR compiling a throwaway effect before the readiness check, and |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
Popov72
left a comment
There was a problem hiding this comment.
That looks good to me.
The only thing that bothers me a little—though it’s not specific to this pull request—is that we’re checking the same elements multiple times, since we’re checking them by mesh/submesh, even though the readiness status doesn’t depend on the mesh/submesh. In this pull request, it’s about lights, but there are also textures and probably other resources...
Maybe something to think about.
Fix material readiness to gate on light texture readiness
[Created by Copilot on behalf of @bghgary]
Problem
SpotLight projection textures and IES profile textures were not gated by material readiness checks. When a projection texture was still loading,
prepareLightSpecificDefineswould setPROJECTEDLIGHTTEXTUREtofalse, the shader would compile without the projection effect, and the material would reportisReady() = true. This causedscene.executeWhenReady()to fire before the projection texture loaded, producing incorrect rendering (e.g. intermittent visual-test failures in BabylonNative).Root cause
SpotLight.prepareLightSpecificDefines()treats unloaded projection/IES textures as "not present" rather than "not ready".scene.isReady()→mesh.isReady()→material.isReadyForSubMesh()all returned true despite the texture still loading.Fix
Light: addsareLightTexturesReady(): boolean(defaulttrue) for subclasses that use texture resources.SpotLight: overridesareLightTexturesReadyand checks_projectionTexture.isReadyOrNotBlocking()and_iesProfileTexture.isReadyOrNotBlocking()(respectsBaseTexture.isBlockingsemantics, matching how other material textures are gated).AreLightsTexturesReadyhelper (new, inmaterialHelper.functions.ts): iteratesmesh.lightSourcesup tomaxSimultaneousLights, respectsscene.lightsEnabledanddisableLighting, and mirrors the light selection used byPrepareDefinesForLights/BindLights.StandardMaterial,BackgroundMaterial,PBRBaseMaterial,OpenPBRMaterial,NodeMaterial): each callsAreLightsTexturesReady(...)inisReadyForSubMeshand returnsfalseearly if any selected light is not ready. ForPBRBaseMaterialandOpenPBRMaterialthe gate is placed before_prepareEffect()so we don't compile and cache a throwaway shader while textures are still loading.LightBlock,PBRMetallicRoughnessBlock): when an explicitthis.lightis bound, the block'sisReadynow checksthis.light.areLightTexturesReady().LightBlockregisters itself as a blocking block when it has an explicit light, so the check participates inNodeMaterial._sharedData.blockingBlocks.When a projection/IES texture finishes loading, the existing
_markMeshesAsLightDirty()callback inSpotLight'sprojectionTexture/iesProfileTexturesetters triggers re-evaluation: the helper returnstrueon the next readiness pass, the material reports ready, andscene.executeWhenReady()proceeds.Tests
packages/dev/core/test/unit/Lights/babylon.spotLight.test.tsadds three regression tests:scene.isReady()staysfalsewhile any light reports its textures are not ready (monkey-patcheslight.areLightTexturesReady).projectionTexture:scene.isReady()staysfalsewhile the projection texture is not ready.iesProfileTexture:scene.isReady()staysfalsewhile the IES profile texture is not ready.All three fail on
masterand pass on this branch. See #18336 for the test-only variant that demonstrates the master failure directly in CI.Related